Add authentication to the queue - #86
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
==========================================
+ Coverage 98.75% 98.87% +0.11%
==========================================
Files 18 20 +2
Lines 886 976 +90
==========================================
+ Hits 875 965 +90
Misses 11 11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ddbb8c8 to
6a11bf4
Compare
6a11bf4 to
11f38ab
Compare
3ea92dc to
02a6eae
Compare
| authorised = [Depends(whitelist_check)] if whitelist_check else None | ||
| router = APIRouter() | ||
|
|
||
| @router.get("/config") |
There was a problem hiding this comment.
Could: Why is this not locked down? I'm not sure if it should be or not but it's odd to be left out
There was a problem hiding this comment.
My thinking was that if someone was locked out they could get config to see if their fedid was in the whitelist (and see who to ask about it). But not really sure either way, happy to change it.
| router = APIRouter() | ||
|
|
||
| @router.get("/config") | ||
| def get_config() -> AppConfig: |
There was a problem hiding this comment.
Should: Maybe depends on the decision about config being locked or not but it would be good to make it the default that all these routes are locked down:
router = APIRouter(dependencies=authorised)
...
@router.patch("/queue/state")This means you're less likely to get the situation where someone adds a route in here thinking it's protected and forgetting to add the dependency
There was a problem hiding this comment.
Yeah, I think I'll just lock down get_config and then can apply it to the whole router
| async def test_add_tasks_to_queue_adds_user_to_task_object( | ||
| app: FastAPI, task_queue_with_history: TaskQueue | ||
| ): | ||
| user = User(fedid="abc12345", email="joe.blogs@diamond.ac.uk", name="Joe Blogs") |
There was a problem hiding this comment.
Must: It's actually Joe Bloggs https://en.wiktionary.org/wiki/Joe_Bloggs, instant PR rejection...
There was a problem hiding this comment.
Haha, I was appeasing my spell checker, forgetting that blog is a word not a name
2 participants
Fixes #2
Adds authentication to the queue. Another ticket should be created to improve authorisation, taking account what visits users have access to etc. For now we just have a whitelist of approved fedIDs.
The user who queued a task is now also added to the task object, so this can now be displayed in the UI.
Instructions on how to test:
daq-queuing-service --config tests/system_tests/config_with_auth.yaml -p 8001 --devhealthzandget_queue_state, and the messagenot authenticatedis returnedInspectKEYCLOAK_TOKENkeyTesting authorisation:
tests/system_tests/config_with_auth.yamlget_config, you get a not authorised error.authorisation_whitelist